-
Notifications
You must be signed in to change notification settings - Fork 573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
interfaces/builtin: allow desktop-launch on Core #14193
Conversation
Despite userd being unusable on Core, access to app metadata is still desirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this seems fine. What kind of metadata are you looking to access?
If it's .desktop files, I wish desktop-launch
granted access to /var/lib/snapd/desktop/applications/*.desktop
instead, as this is where all the nice cleaned .desktop files are with additional snapd-inserted metadata. The ones in /snap/*/*/meta/gui/*.desktop
have e.g. relative paths to icon files. But I guess this isn't really a problem for this PR...
@olivercalder it does that: And yeah, we're after the cleaned-up desktop files, and the icons pointed from there. Ref.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, It would be very confusing to have desktop-launch on core that cannot be used to launch snaps.
I believe it would be a better to have a new interface dedicated to only allow accessing snaps' metadata to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, this makes sense because Core Desktop requires it. But I think that @jhenstridge should also review it.
@ZeyadYasser the interface allows access to a DBus endpoint, the fact that there's nothing to serve that endpoint is just because there isn't a user session on Core. Same as if you had a Classic system with no session running. So I don't think one precludes the other. In fact, you could run a user session on Core and it would then work just fine. And Core Desktop is another approach solving the same. I suppose the comment should just go away. |
Ping? Any thoughts on whether there's anything missing here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure I might be missing some context, but I am more inclined for a separate interface that just allows access to snaps metadata (e.g. desktop-metadata
) if that's is all what is needed instead of growing desktop-launch
to other use cases to avoid the confusion of giving a false sense of currently supporting desktop launch on core when it is not supported yet.
@pedronis @zyga would love to know you input and suggestions
@@ -90,6 +90,7 @@ func (iface *desktopLaunchInterface) Name() string { | |||
func (iface *desktopLaunchInterface) StaticInfo() interfaces.StaticInfo { | |||
return interfaces.StaticInfo{ | |||
Summary: desktopLaunchSummary, | |||
ImplicitOnCore: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this would only make sense for core desktop? even though it is not yet supported which is my other concern.
ImplicitOnCore: true, | |
ImplicitOnCore: release.OnCoreDesktop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, most SnapD interfaces only allow access to resources, whether they exist on a system or not. As documented above, you can have a user session, with userd running, on Core. You just don't, by default. Same if you install Ubuntu Server.
IMO the reason to split interfaces out is only if you want to allow a subset of what the existing one provides - not whether they're used or not. So yes, if you wanted a snap to only have access to app metadata, but not allowing it to launch them, then you'll need separate interfaces. Otherwise we'll grow the number of interfaces quicker than necessary.
if the reason to use this interface is to get access to the desktop app metadata, we should really introduce a separate interface that is only about being able to read that data |
Despite userd being unusable on Core, access to app metadata is still desirable.